Skip to content

feat(github): share installations with web Cloud Agents - #5978

Open
pandemicsyn wants to merge 40 commits into
mainfrom
feat/github-connections-pr2
Open

pandemicsyn wants to merge 40 commits into
mainfrom
feat/github-connections-pr2

Conversation

@pandemicsyn

@pandemicsyn pandemicsyn commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Outcome

Default-off shared canonical GitHub installations for WEB Cloud Agents with exact tenant-association pinning. Slack shared use remains PR3; Code Reviewer shared routing remains PR4.

Reviewed base: current main at cf59d8662499749228164232615da5af0abe5134. Reviewed head: af1734d3ce0b8d11acad16622754be8618603cdb.

Exact Association Boundaries

Generic token and Chat SDK webhook paths require canonical sharing_mode = exclusive. Managed shared lookup requires an exact active association ID. GitHub bot-link signed state carries exact platformIntegrationId; callback validates association, installation, Standard/Lite identity, canonical lifecycle, local health, and current Kilo owner or organization membership.

Predeploy bot-link state without an association ID is accepted only while management and sharing admission remain disabled and only for one healthy canonical-exclusive or true-legacy literal-null app association. Explicit Standard rows require exact signed association identity. Keep features disabled for at least one signed-state TTL after deployment.

Simple Synchronous Lifecycle

Required canonical/local database cleanup runs first and fails non-2xx without recording completion. Standard bot unlink runs second as best-effort alerting. Completed-success receipts dedupe later deliveries; concurrent and crash-window repeats are idempotent. GitHub does not automatically retry failed webhooks, so manual redelivery after database failure is the accepted recovery. No lifecycle queue, cron, outbox, lease, fencing, Retry-After, or automatic retry worker exists.

Repository refresh follows lifecycle lock order and cannot revive terminal state.

Provider Reservation Boundary

Slack, Linear, and Discord OAuth attempts are durable, expiring, owner-row locked, purpose-bound, and single-use. PR3 owns provider-installation generation and lazy recovery.

Migration And Rollout

Generated migration: 0243_exotic_triton.sql. Replacement indexes build concurrently with required transaction boundaries. Before enabling sharing, stale eligible-unlinked must be zero. PR2 adds no canonical reconciliation DML.

Verification

  • Accepted pre-rebase bot-link/state/DB resolver suite: 99 passed
  • Accepted pre-rebase schema/migration suite: 101 passed; fresh bootstrap passed
  • Final-rebase Git token service: 633 passed
  • Final-rebase focused Cloud Agent suites: 429 passed
  • Final-rebase range-diff: all 28 PR2 commits exact
  • Changes-only typecheck and web/DB/token/Cloud Agent lint: passed
  • Format, whitespace, and conflict-marker checks: passed

The final-rebase PostgreSQL-backed Jest/bootstrap rerun was blocked before test execution because the local OrbStack Docker daemon was unresponsive; docker info, Compose health, and pnpm test:db timed out. This was not an assertion failure. The accepted pre-rebase head had the schema/bootstrap and focused web suites passing.

No production queries, repairs, associations, settings, flags, backfills, deployments, or external provider operations were performed.

Base automatically changed from feat/github-connections-pr1 to main September 8, 2026 21:08
@pandemicsyn
pandemicsyn force-pushed the feat/github-connections-pr2 branch 2 times, most recently from fa6817f to d70becc Compare September 9, 2026 16:36
@pandemicsyn
pandemicsyn marked this pull request as ready for review September 9, 2026 17:39
@kilo-code-bot

kilo-code-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

The migration-only incremental changes (0243 pre-flight orphan guard, the 0244 split, the retry-safe concurrent index rebuild in 0245, and the 0246 index drops plus sharing_mode check) follow existing migration conventions; the one outstanding finding is the fail-closed candidate guard in findGitHubBotLinkIntegrations, which counts usable rows after an unordered .limit(2).

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/db/platform-integrations.ts 96 .limit(input.platformIntegrationId ? 1 : 2) runs before the JS usability filter and without ORDER BY, so the callback's usableCandidates.length === 1 fail-closed check can accept an ambiguous bot link when 3+ associations match.

Note: the inline comment for this finding could not be attached because a stale PENDING review (id 5158007287, commit d70becc) on this PR blocks new review creation.

Files Reviewed (5 files)
  • packages/db/src/migrations/0243_reconcile_orphaned_github_installation_id.sql - no issues
  • packages/db/src/migrations/0244_salty_cyclops.sql - no issues
  • packages/db/src/migrations/0245_calm_lockheed.sql - no issues
  • packages/db/src/migrations/0246_clean_the_renegades.sql - no issues
  • apps/web/src/lib/integrations/db/platform-integrations.ts - 1 issue

Fix these issues in Kilo Cloud

Previous Review Summaries (15 snapshots, latest commit 12b0e56)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 12b0e56)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

The latest commit's three fixes (personal GitHub guard scoping, tRPC output schemas, and association detail resync) are correct; the one remaining finding is the fail-closed candidate guard in findGitHubBotLinkIntegrations, which counts usable rows after an unordered .limit(2).

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/db/platform-integrations.ts 96 .limit(input.platformIntegrationId ? 1 : 2) runs before the JS usability filter and without ORDER BY, so the callback's usableCandidates.length === 1 fail-closed check can accept an ambiguous bot link when 3+ associations match. Move the filters into SQL or count usable rows over the full candidate set.

Note: the inline comment for this finding could not be attached because a stale PENDING review (id 5158007287, commit d70becc) on this PR blocks new review creation.

Files Reviewed (8 files)
  • apps/web/src/components/cloud-agent-next/NewSessionPanel.tsx - no issues
  • apps/web/src/lib/integrations/db/platform-integrations.ts - 1 issue
  • apps/web/src/routers/cloud-agent-next-router.ts - no issues
  • apps/web/src/routers/cloud-agent-next-router.test.ts - no issues
  • apps/web/src/routers/github-apps-router.ts - no issues
  • apps/web/src/routers/github-apps-router.test.ts - no issues
  • apps/web/src/routers/organizations/organization-cloud-agent-next-router.ts - no issues
  • apps/web/src/routers/organizations/organization-cloud-agent-next-router.test.ts - no issues

Fix these issues in Kilo Cloud

Previous review (commit 242eb98)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

The dedupe fix in the changed commit is correct and the two prior webhook/OAuth-attempt fixes remain verified; the previously reported fail-closed guard in the GitHub bot-link resolver (findGitHubBotLinkIntegrations) is still unresolved at HEAD.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/db/platform-integrations.ts 96 .limit(input.platformIntegrationId ? 1 : 2) runs before the JS usability filter, so with 3+ matching associations the arbitrary pair (no ORDER BY) can contain exactly one usable row and the callback's usableCandidates.length === 1 guard accepts an ambiguous bot link instead of failing closed. Move the candidate filters into SQL or drop the limit and count in JS.
Files Reviewed (4 files)
  • apps/web/src/lib/cloud-agent/github-integration-helpers.ts - dedupe fix verified (no issues)
  • apps/web/src/lib/cloud-agent/github-integration-helpers.test.ts - tests added (no issues)
  • apps/web/src/lib/integrations/db/platform-integrations.ts - 1 issue (carried forward, unchanged this commit)
  • apps/web/src/app/api/integrations/github/callback/route.ts - caller re-verified (no issues)

Fix these issues in Kilo Cloud

Previous review (commit 731e775)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

Incremental review verified both fixes in this commit (webhook receipts are now recorded only on response.ok, and an unexpired pending OAuth attempt is superseded by a new attempt); one previously reported, still-unresolved fail-closed guard in the bot-link resolver remains.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/db/platform-integrations.ts 96 .limit(2) runs before the JS usability filter, so with three or more matching associations the arbitrary pair (no ORDER BY) can contain exactly one usable row and the callback's usableCandidates.length === 1 guard accepts an ambiguous bot link instead of failing closed. Move the candidate filters into SQL or drop the limit and count in JS.
Files Reviewed (5 files)
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.ts - fix verified (receipt recorded only on response.ok)
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.test.ts - tests added
  • apps/web/src/lib/integrations/provider-oauth-attempts.ts - fix verified (pending attempts superseded)
  • apps/web/src/lib/integrations/provider-oauth-attempts.test.ts - tests added
  • apps/web/src/lib/integrations/db/platform-integrations.ts - 1 issue (carried forward, unchanged this commit)

Fix this issue in Kilo Cloud

Previous review (commit ec51eee)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

Incremental review of the migration-hardening commit found no new issues in the changed code; the previously reported .limit(2)-before-filter ambiguity guard in the bot-link resolver remains unresolved at HEAD.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/db/platform-integrations.ts 96 .limit(2) runs before the JS usability filter, so with three or more matching associations the arbitrary pair (no ORDER BY) can contain exactly one usable row and the callback's usableCandidates.length === 1 guard accepts an ambiguous bot link instead of failing closed. Move the candidate filters into SQL or drop the limit and count in JS.
Files Reviewed (7 files)
  • packages/db/src/migrations/0243_reconcile_orphaned_github_installation_id.sql
  • packages/db/src/migrations/0244_free_thena.sql
  • packages/db/src/migrations/meta/_journal.json
  • packages/db/src/migrations/meta/0243_snapshot.json (generated)
  • packages/db/src/migrations/meta/0244_snapshot.json (generated)
  • packages/db/src/schema.ts
  • apps/web/src/lib/integrations/db/platform-integrations.ts - 1 issue (carried forward, unchanged this commit)

Fix this issue in Kilo Cloud

Previous review (commit af1734d)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

findGitHubBotLinkIntegrations applies .limit(2) before its JS candidate filter, so the bot-link "exactly one healthy association" ambiguity guard can fail open when three or more associations share an installation.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/db/platform-integrations.ts 96 .limit(2) runs before the JS usability filter, so with three or more matching associations the arbitrary pair (no ORDER BY) can contain exactly one usable row and usableCandidates.length === 1 accepts an ambiguous bot link instead of failing closed. Move the candidate filters into SQL or drop the limit and count in JS.
Files Reviewed (5 files)
  • apps/web/src/lib/integrations/db/platform-integrations.ts - 1 issue
  • apps/web/src/lib/bot/github-link-state.ts
  • apps/web/src/app/api/integrations/github/callback/route.ts
  • apps/web/src/app/github/link/route.ts
  • apps/web/src/lib/integrations/github/multiple-installations.ts

Fix this issue in Kilo Cloud

Inline publication was blocked: this PR has an existing bot pending review (id 5158007287) that this session is not permitted to clear, so the finding is recorded here.

Previous review (commit fda919a)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

findGitHubBotLinkIntegrations applies .limit(2) before its JS candidate filter, so the bot-link "exactly one healthy association" ambiguity guard can fail open when three or more associations share an installation.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/db/platform-integrations.ts 96 .limit(2) runs before the JS usability filter, so an arbitrary pair (no ORDER BY) can contain exactly one usable row and the callback's usableCandidates.length === 1 guard accepts even though multiple healthy associations exist. Move the candidate filters into SQL or drop the limit and count in JS.
Files Reviewed (9 files)
  • apps/web/src/app/api/integrations/github/callback/route.ts
  • apps/web/src/app/api/integrations/github/callback/route.test.ts
  • apps/web/src/app/github/link/route.ts
  • apps/web/src/app/github/link/route.test.ts
  • apps/web/src/lib/bot/github-link-state.ts
  • apps/web/src/lib/bot/github-link-state.test.ts
  • apps/web/src/lib/integrations/db/platform-integrations.ts - 1 issue
  • apps/web/src/lib/integrations/db/platform-integrations.test.ts
  • apps/web/src/lib/integrations/github/multiple-installations.ts

Fix this issue in Kilo Cloud

Inline publication was blocked: this PR has an existing bot pending review (id 5158007287) that this session is not permitted to clear, so the finding is recorded here.

Previous review (commit 4c3d38b)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Rebase onto current main left GitHub-installation review scope unchanged; overlapping files differ only by already-landed main content, with high confidence.

Files Reviewed (3 files)
  • ENVIRONMENT.md
  • apps/web/src/lib/integrations/db/platform-integrations.test.ts
  • apps/web/src/lib/integrations/db/platform-integrations.ts

Previous review (commit 98af34e)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Lock-observation tests now match the exact advisory-lock SQL already used by sibling suites; the single-file incremental change looks correct with high confidence.

Files Reviewed (1 file)
  • apps/web/src/lib/integrations/db/github-installations.test.ts

Previous review (commit fbc73e0)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Lifecycle lock-overlap tests now wait for a blocked owner advisory lock before releasing the holder; the single-file incremental change looks correct with high confidence.

Files Reviewed (1 file)
  • apps/web/src/lib/integrations/db/github-installations.test.ts

Previous review (commit f0df914)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Repository refresh now follows the lifecycle lock order and skips deleted or inactive projections; the two-file incremental change looks correct with high confidence.

Files Reviewed (2 files)
  • apps/web/src/lib/integrations/db/github-installations.test.ts
  • apps/web/src/lib/integrations/db/platform-integrations.ts

Previous review (commit 622241e)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Synchronous GitHub lifecycle cleanup now records completed receipts only after required DB work; the incremental simplification looks correct with high confidence.

Files Reviewed (8 files)
  • apps/web/src/lib/integrations/db/github-installations.test.ts
  • apps/web/src/lib/integrations/db/github-installations.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-handler.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-handler.ts
  • packages/db/src/migrations/0243_exotic_triton.sql
  • packages/db/src/schema.ts

Previous review (commit cd4e7d7)

Status: No Issues Found | Recommendation: Merge

Executive Summary

In-lease GitHub lifecycle receipts now return retryable 503 with Retry-After; the five-file incremental change looks correct with high confidence.

Files Reviewed (5 files)
  • ENVIRONMENT.md
  • apps/web/src/lib/integrations/db/github-installations.test.ts
  • apps/web/src/lib/integrations/db/github-installations.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.ts

Previous review (commit f1bbbb4)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

Failed GitHub installation.deleted retries can still be acknowledged as duplicates while a pending receipt lease is held, so unlink/cleanup is never reclaimed.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0

Fix these issues in Kilo Cloud

Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/platforms/github/webhook-handler.ts 184 In-lease pending receipts return HTTP 200 Duplicate, so GitHub stops retrying after unlink+release failures
Files Reviewed (5 files)
  • apps/web/src/lib/integrations/db/github-installations.ts
  • apps/web/src/lib/integrations/github/git-token-service.behavior.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.ts - 1 issue
  • services/git-token-service/src/installation-lookup-service.ts

Previous review (commit 458e94b)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

Failed GitHub installation.deleted retries can still be acknowledged as duplicates while a pending receipt lease is held, so unlink/cleanup is never reclaimed.

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0

Fix these issues in Kilo Cloud

Issue Details (click to expand)

WARNING

File Line Issue
apps/web/src/lib/integrations/platforms/github/webhook-handler.ts 184 In-lease pending receipts return HTTP 200 Duplicate, so GitHub stops retrying after unlink+release failures
Files Reviewed (19 files)
  • ENVIRONMENT.md
  • apps/web/src/app/api/integrations/github/callback/route.ts
  • apps/web/src/app/api/integrations/linear/callback/route.test.ts
  • apps/web/src/lib/integrations/db/github-installations.test.ts
  • apps/web/src/lib/integrations/db/github-installations.ts
  • apps/web/src/lib/integrations/github/runtime-authorization.test.ts
  • apps/web/src/lib/integrations/github/runtime-authorization.ts
  • apps/web/src/lib/integrations/oauth-state.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.ts - 1 issue
  • apps/web/src/lib/integrations/provider-oauth-attempts.ts
  • apps/web/src/lib/organizations/organizations.ts
  • apps/web/src/lib/user/index.ts
  • packages/db/src/migrations/0243_gray_supernaut.sql
  • packages/db/src/migrations/meta/0243_snapshot.json
  • packages/db/src/migrations/meta/_journal.json
  • packages/db/src/schema.ts
  • services/git-token-service/src/installation-lookup-service.test.ts
  • services/git-token-service/src/installation-lookup-service.ts

Previous review (commit d70becc)

Status: 3 Issues Found | Recommendation: Address before merge

Executive Summary

Exclusive GitHub uninstall retries can skip cleanup after unlink failures, unpinned runtime auth stays broken after sharing demotion, and the migration rebuilds live platform_integrations unique indexes without CONCURRENTLY.

Overview

Severity Count
CRITICAL 0
WARNING 3
SUGGESTION 0

Fix these issues in Kilo Cloud

Issue Details (click to expand)

WARNING

File Line Issue
packages/db/src/migrations/0243_eager_taskmaster.sql 41 Blocking unique index rebuild on populated platform_integrations without CONCURRENTLY
apps/web/src/lib/integrations/github/runtime-authorization.ts 119 Unpinned runtime auth treats disconnected sibling rows as ambiguous after sharing demotion
apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-handler.ts 190 Rethrowing unlink failures skips exclusive uninstall cleanup on GitHub retry
Files Reviewed (50 files)
  • ENVIRONMENT.md
  • apps/web/src/app/api/integrations/bitbucket/connect/route.test.ts
  • apps/web/src/app/api/integrations/github/callback/route.ts
  • apps/web/src/app/api/integrations/linear/callback/route.test.ts
  • apps/web/src/app/api/integrations/linear/connect/route.test.ts
  • apps/web/src/components/cloud-agent-next/NewSessionPanel.tsx
  • apps/web/src/components/integrations/GitHubIntegrationDetails.tsx
  • apps/web/src/components/integrations/OrganizationGitHubInstallations.tsx
  • apps/web/src/components/shared/RepositoryCombobox.tsx
  • apps/web/src/lib/admin/github-installation-uninstall.test.ts
  • apps/web/src/lib/admin/github-installation-uninstall.ts
  • apps/web/src/lib/agent-config/db/agent-configs.ts
  • apps/web/src/lib/cloud-agent/github-integration-helpers.test.ts
  • apps/web/src/lib/cloud-agent/github-integration-helpers.ts
  • apps/web/src/lib/code-reviews/review-memory/settings.ts
  • apps/web/src/lib/integrations/db/github-installations.test.ts
  • apps/web/src/lib/integrations/db/github-installations.ts
  • apps/web/src/lib/integrations/db/platform-integrations.test.ts
  • apps/web/src/lib/integrations/db/platform-integrations.ts
  • apps/web/src/lib/integrations/github-apps-service.ts
  • apps/web/src/lib/integrations/github/multiple-installations.test.ts
  • apps/web/src/lib/integrations/github/multiple-installations.ts
  • apps/web/src/lib/integrations/github/runtime-authorization.test.ts
  • apps/web/src/lib/integrations/github/runtime-authorization.ts - 1 issue
  • apps/web/src/lib/integrations/github/sharing-compatibility.ts
  • apps/web/src/lib/integrations/oauth-state.test.ts
  • apps/web/src/lib/integrations/oauth-state.ts
  • apps/web/src/lib/integrations/oauth/common.ts
  • apps/web/src/lib/integrations/oauth/platforms/discord-callback.ts
  • apps/web/src/lib/integrations/oauth/platforms/linear-callback.ts
  • apps/web/src/lib/integrations/oauth/platforms/slack-callback.ts
  • apps/web/src/lib/integrations/platforms/github/adapter.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handler.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-handler.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-handler.ts - 1 issue
  • apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-target-handler.test.ts
  • apps/web/src/lib/integrations/platforms/github/webhook-handlers/installation-target-handler.ts
  • apps/web/src/lib/integrations/provider-oauth-attempts.test.ts
  • apps/web/src/lib/integrations/provider-oauth-attempts.ts
  • apps/web/src/lib/organizations/organizations.ts
  • apps/web/src/lib/security-agent/db/security-config.ts
  • apps/web/src/lib/user/index.test.ts
  • apps/web/src/lib/user/index.ts
  • apps/web/src/routers/github-apps-router.test.ts
  • apps/web/src/routers/github-apps-router.ts
  • packages/db/src/migrations/0243_eager_taskmaster.sql - 1 issue
  • packages/db/src/schema.test.ts
  • packages/db/src/schema.ts
  • services/git-token-service/src/installation-lookup-service.test.ts
  • services/git-token-service/src/installation-lookup-service.ts

Reviewed by deepseek-v4.1-flash · Input: 271.7K · Output: 44.4K · Cached: 1.4M

Review guidance: REVIEW.md from base branch main

@pandemicsyn
pandemicsyn force-pushed the feat/github-connections-pr2 branch from 98af34e to 4c3d38b Compare September 10, 2026 15:26
@pandemicsyn
pandemicsyn force-pushed the feat/github-connections-pr2 branch from fda919a to af1734d Compare September 11, 2026 01:27

@iscekic iscekic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to unblock

Comment thread apps/web/src/lib/integrations/platforms/github/webhook-handler.ts
Comment thread apps/web/src/lib/integrations/provider-oauth-attempts.ts
Comment thread apps/web/src/lib/cloud-agent/github-integration-helpers.ts Outdated
Comment thread packages/db/src/migrations/0243_exotic_triton.sql Outdated
@pandemicsyn
pandemicsyn added this pull request to stack #6140 September 14, 2026 13:58
@pandemicsyn pandemicsyn changed the title feat(github): share installations with web Cloud Agents (2/4) feat(github): share installations with web Cloud Agents Sep 14, 2026
Split 0243 into a pre-flight reconciliation migration and the
FK-adding migration:

- 0243_reconcile_orphaned_github_installation_id.sql nulls out any
  platform_integrations.github_installation_id that no longer
  references an existing github_app_installations row, so the FK
  added below cannot fail its full-table validation on drift.
- 0244_free_thena.sql (renamed from 0243_exotic_triton.sql) now
  declares the FK with an explicit ON DELETE restrict, matching the
  repo convention for blocking FKs and the lifecycle already
  enforced in code: github_app_installations rows are soft-deleted
  (lifecycle_state/deleted_at), and the only hard-delete path
  (anonymizeCloudUserData) already guards with a NOT EXISTS check
  against remaining associations.

schema.ts documents the ON DELETE restrict choice inline.
dispatchSharedOnce recorded a completed delivery receipt whenever the
shared-installation receipt lookup wasn't missing_canonical,
regardless of the dispatched handler's response status. A non-2xx
dispatch was therefore marked completed, so a later GitHub
redelivery of the same event was short-circuited as a duplicate and
could never retry.

Only record the receipt when response.ok, leaving non-2xx dispatches
unrecorded so GitHub's redelivery can retry them. missing_canonical
behavior is unchanged.
beginProviderOAuthAttempt only expired pending/consumed rows whose
expires_at was already in the past before inserting the new attempt.
An unexpired pending row for the same owner+provider (e.g. an
abandoned flow) still collided with the partial unique index on
(owner, provider, status = 'pending'), blocking a fresh /connect
attempt for up to ATTEMPT_TTL_MS (10 minutes) and surfacing
oauth_init_failed.

Drop the expires_at filter for pending rows so a new attempt always
supersedes an existing pending one for that owner+provider, within
the same tx and owner-row lock already held. Consumed rows are still
only expired once actually past their TTL, since they don't hold
that unique slot.

consumeProviderOAuthAttempt's pre-step doesn't insert and matches the
row to consume by exact state_hash, so it doesn't share this
collision pattern and is unchanged.
results.flatMap(r => r.repositories) in fetchRepositoriesForIntegrations
had no dedup after dedupeRepositories was removed, so a corrupted or
duplicated cached repositories array on a single installation would
surface the same repo twice for that installation.

Deliberately keep one entry per (platformIntegrationId, id) rather
than deduping by repo id alone: NewSessionPanel already keys and
selects repository rows by platform + platformIntegrationId + id, and
the existing 'preserves both association choices when two
installations expose one repository' test documents that two
different installations granting the same repo must both remain
selectable, since session start resolves the token/permissions from
the chosen installation. Deduping by repo id would silently drop one
of those legitimate, functionally distinct entries.

Added a test covering the same-installation duplicate case and
confirmed the existing cross-installation test is unaffected.
Resolved apps/web/src/components/cloud-agent-next/NewSessionPanel.tsx
by taking main's refactored submit-path structure (hoisted
creationIntent/creationInput/bitbucketRepo memos, firstChatCreationOperation
as state) while restoring PR2's shared-GitHub-installation additions:

- RepositoryOption.githubAppType field
- githubAppType mapping onto GitHub repo options
- explicit "Select the GitHub repository again" guard before creationIntent
  is used in handleStartSession
- githubAppType included in the repo command-item search value
- the "Lite" badge for lite GitHub App installations

ENVIRONMENT.md auto-merged cleanly.
Three must-fix issues found in code review of the GitHub connections
work:

1. NewSessionPanel's GitHub-integration guard was unconditional on
   selectedGitHubIntegrationId, but that field is only ever populated
   for organization-scoped repos. Every personal (non-org) user
   selecting a GitHub repo hit 'Select the GitHub repository again'
   and could never start a session. Scope the guard to
   organizationId, matching the sibling conditional already used for
   creationIntent.

2. Both listGitHubRepositories tRPC output schemas silently stripped
   githubAppType (and the personal router also stripped
   platformIntegrationId/platformAccountLogin), making the new 'Lite'
   badge dead code and contributing to (1). Added the missing fields
   to both schemas.

3. refreshInstallation stopped syncing account/permissions/scopes/
   repository_access/installed_at onto the platform_integrations
   association row after the shared-installation refactor removed the
   legacy upsertPlatformIntegrationForOwner call. getInstallation/
   listIntegrations read those columns directly off
   platform_integrations, so GitHubIntegrationDetails kept showing
   stale data after a refresh. Added
   syncIntegrationInstallationDetails, scoped by integration id only
   (safe for shared/multi-owner installations, unlike the removed
   legacy writer), and wired it into refreshInstallation.

Added regression tests for all three: both listGitHubRepositories
routers now assert a full repo object round-trips unchanged through
the real tRPC output validation, and refreshInstallation's tests
assert syncIntegrationInstallationDetails is called with fresh data
(and not called on the early-exit failure path).
…liation

Split 0244 into three independently-tracked migrations so a failed or
interrupted deploy can never strand durably-committed DDL behind an
untracked migration, and never leaves a window with neither the old
nor the new uniqueness protection in place:

- 0244_salty_cyclops.sql: plain transactional additions only (new
  tables, new columns, all FK constraints including the validating
  platform_integrations FK). No explicit COMMIT/BEGIN, so Postgres
  rolls the whole file back atomically on any failure; a retry is
  always clean.
- 0245_calm_lockheed.sql: only the four CREATE UNIQUE INDEX
  CONCURRENTLY replacement indexes, fenced with the repo's approved
  COMMIT/BEGIN pattern. Does NOT drop the old two unique indexes.
- 0246_clean_the_renegades.sql: drops the two old unique indexes and
  adds the sharing_mode CHECK constraint, only after 0245 has been
  independently tracked as applied, so the replacements are always
  confirmed built before the originals are removed.

Verified empirically: applying only through 0245 leaves both the old
and new indexes coexisting (all valid) - there is no point in the
sequence where the uniqueness invariant is unprotected. 0246 then
applies cleanly as its own later migration.

Per packages/db/AGENTS.md, deliberately did not add IF NOT EXISTS or
a pre-emptive DROP INDEX IF EXISTS around the CONCURRENTLY builds:
the skill explicitly prohibits automatically masking a failed/invalid
concurrent index build that way and requires explicit operator
inspection and recovery (DROP INDEX CONCURRENTLY / REINDEX INDEX
CONCURRENTLY) instead. The file split already achieves retry-safety
at the migration-tracking level without needing that.

Also hardens 0243: replace the silent UPDATE that nulled any
dangling platform_integrations.github_installation_id with a DO 12662
guard that RAISEs if any orphan exists. Nulling was a behavior change
in disguise - for a row with github_app_type IS NULL, clearing the
reference makes it newly eligible for the legacy bot-link fallback
lookup (which treats github_app_type = 'standard' OR IS NULL as
equivalent). Since production orphan_count is confirmed 0, assert the
invariant instead of silently mutating data/eligibility.

Verified: drizzle-kit check passes; the full chain (including a
staged partial-apply through 0245 only, then 0246 separately) applies
cleanly from scratch against a fresh local Postgres; schema.ts is
unchanged (same end-state schema, just staged across more granular
migrations); packages/db/src/schema.test.ts (103 tests) passes.
Each of the four CREATE UNIQUE INDEX CONCURRENTLY statements in
0245_calm_lockheed.sql had no idempotency guard. CONCURRENTLY can
leave an INVALID index under its name if a build fails partway or is
interrupted; a blind retry of CREATE UNIQUE INDEX CONCURRENTLY on the
same name then fails with 'already exists' instead of rebuilding it -
the exact non-retryable strand Finding #1 addressed, just moved down
to the index-build step.

Precede each build with DROP INDEX CONCURRENTLY IF EXISTS on its
exact name: a no-op on the happy path (index doesn't exist yet), and
a clean removal of a leftover invalid index otherwise, so every retry
converges to the same end state. Per packages/db/AGENTS.md, deliberately
not using CREATE ... CONCURRENTLY IF NOT EXISTS, since that would leave
a leftover invalid index in place unrebuilt instead of recovering it;
DROP INDEX CONCURRENTLY is the exact recovery mechanism the skill
names, just applied proactively rather than only after manual
diagnosis. Both DROP CONCURRENTLY and CREATE CONCURRENTLY remain in
the existing post-COMMIT/pre-BEGIN section, outside any transaction
block, as required. No change to the final index set, names, or
schema.ts/snapshots.

Verified: drizzle-kit check passes. Full chain applies cleanly from
scratch. Directly simulated the failure mode: seeded a duplicate-data
conflict, forced one of the four builds to fail and leave an INVALID
index (confirmed indisvalid=false), confirmed a blind retry without
this guard fails with 'already exists' (reproducing the bug), then
repaired the data and confirmed the guarded migration now retries
cleanly to a fully valid final index set. packages/db/src/schema.test.ts
(103 tests) passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants